Update SDK to bring in RSG one assembly changes #30827

captainsafia · 2021-03-11T00:50:47Z

No description provided.

captainsafia · 2021-03-16T16:34:27Z

Closing in favor of merging this into #30891.

captainsafia · 2021-03-17T22:45:44Z

...AD/Authentication.AzureADB2C.UI/src/Microsoft.AspNetCore.Authentication.AzureADB2C.UI.csproj

    <AddRazorSupportForMvc>true</AddRazorSupportForMvc>
+    <NoWarn>RS0016</NoWarn>


The build for this codepath relies on the RazorPageDocumentClassifier which we didn't update to support consolidated views. As a result, the types generated in the output assembly are still public and picked up by the public API analyzer during the build.

I fixed this in this PR but we'll need to wait for the aspnetcore -> SDK -> installer flow with this change to finish before we can remove this check.

Fair enough. I would block on that, but it's good that we picked it up quickly

captainsafia · 2021-03-17T22:46:37Z

src/Razor/Microsoft.AspNetCore.Mvc.Razor.Extensions/src/PublicAPI.Unshipped.txt

Eh, since we're doing this in multiple classes now I think using a constructor that takes a flag will be better than duplicating the code to avoid ragrets later.

captainsafia · 2021-03-17T22:50:54Z

src/SignalR/clients/ts/yarn.lock

@@ -0,0 +1,4 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


yarn expects there to be a lock file even it if is empty. Commiting this to avoid the annoyance of always having this as an untracked file.

pranavkm · 2021-03-17T22:48:52Z

...AD/Authentication.AzureADB2C.UI/src/Microsoft.AspNetCore.Authentication.AzureADB2C.UI.csproj

    <AddRazorSupportForMvc>true</AddRazorSupportForMvc>
+    <NoWarn>RS0016</NoWarn>


Fair enough. I would block on that, but it's good that we picked it up quickly

pranavkm · 2021-03-17T22:49:20Z

src/Azure/AzureAD/test/FunctionalTests/WebAuthenticationTests.cs

@@ -31,14 +31,8 @@ public WebAuthenticationTests(WebApplicationFactory<Startup> fixture)
        public static TheoryData<string> NotAddedEndpoints =>
            new TheoryData<string>()
            {
-                "/AzureAD/Account/AccessDenied",


What happened here?

So, previously, the Azure AD UI projects used a NullApplicationFactory and instead relied on their own logic to discover the application parts from the related assembly (see https://github.com/dotnet/aspnetcore/pull/31039/files#diff-754ffcfbe4655cc4cca0a446a4f1b0eb3552a8795c136385b6ffad03259e0a7dL199). This API gets invoked from the AddAzureAD extension method.

Since the views are compiled into the app assembly and we are no longer using the NullApplicationFactory, we don't need to rely on the discovery logic (I removed it in the new PR above).

The controllers don't change though because those are discovered with a feature provider that exists outside of the views story.

pranavkm · 2021-03-17T23:01:26Z

src/Identity/UI/src/IdentityBuilderUIExtensions.cs

@@ -47,7 +47,7 @@ public static IdentityBuilder AddDefaultUI(this IdentityBuilder builder)
        private static readonly IDictionary<UIFramework, string> _assemblyMap =
            new Dictionary<UIFramework, string>()
            {
-                [UIFramework.Bootstrap4] = "Microsoft.AspNetCore.Identity.UI.Views.V4",
+                [UIFramework.Bootstrap4] = "Microsoft.AspNetCore.Identity.UI.",


I think this goes away. This used to switch the related part but we're not generating any now.

Yep. You're right. I should've looked way closer at this. I made the appropriate changes in #31039.

pranavkm · 2021-03-17T23:02:37Z

src/Mvc/test/WebSites/ControllersFromServicesWebSite/Startup.cs

-                        .GetRelatedAssemblies(GetType().Assembly, throwOnError: true)
-                        .SingleOrDefault();
-                    foreach (var part in CompiledRazorAssemblyApplicationPartFactory.GetDefaultApplicationParts(relatedAssenbly))
+                    foreach (var part in CompiledRazorAssemblyApplicationPartFactory.GetDefaultApplicationParts(Assembly.GetExecutingAssembly()))


Doesn't the consolidated part factory already take care of this?

It does. However, this particularly startup logic removes all the discovered application parts and adds its own.

aspnetcore/src/Mvc/test/WebSites/ControllersFromServicesWebSite/Startup.cs

Line 26 in 35bb1b8

.ConfigureApplicationPartManager(manager => manager.ApplicationParts.Clear())

I think it does this to validate the behavior of the specific extension methods used here.

pranavkm · 2021-03-17T23:04:19Z

src/Razor/Microsoft.AspNetCore.Mvc.Razor.Extensions/src/RazorExtensions.cs

+                builder.Features.Add(new RazorPageDocumentClassifierPass(useConsolidatedMvcViews: true));
+                builder.Features.Add(new MvcViewDocumentClassifierPass(useConsolidatedMvcViews: true));


Suggested change

builder.Features.Add(new RazorPageDocumentClassifierPass(useConsolidatedMvcViews: true));

builder.Features.Add(new MvcViewDocumentClassifierPass(useConsolidatedMvcViews: true));

builder.Features.Add(new RazorPageDocumentClassifierPass(builder.Configuration.UseConsolidatedMvcViews));

builder.Features.Add(new MvcViewDocumentClassifierPass(builder.Configuration.UseConsolidatedMvcViews));

Don't need the if

pranavkm · 2021-03-17T23:05:05Z

src/SignalR/clients/ts/yarn.lock

@@ -0,0 +1,4 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


@BrennanConroy why do you do this to us?

captainsafia · 2021-03-18T19:21:33Z

@pranavkm I closed this PR yet again since we pulled out the consolidated view stuff. New PR is at #31039.

ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Mar 11, 2021

captainsafia force-pushed the safia/update-sdk-one-asm branch 5 times, most recently from e8e97c7 to 21d3307 Compare March 15, 2021 19:01

captainsafia closed this Mar 16, 2021

captainsafia reopened this Mar 17, 2021

captainsafia force-pushed the safia/update-sdk-one-asm branch 2 times, most recently from 872e461 to e5e3473 Compare March 17, 2021 20:09

captainsafia added 7 commits March 17, 2021 13:50

Update SDK to bring in RSG one assembly changes

755513d

Update version to pick up bug fix

8c2efb6

Update WASM authentication tests

e84fe3b

Remove unneeded assembly attributes file

0e4c0f0

Fix up Startup for test site

63e71c1

Bump to new SDK

c19003c

Clean up consolidated views APIs

4986a10

captainsafia force-pushed the safia/update-sdk-one-asm branch from e5e3473 to 4986a10 Compare March 17, 2021 21:19

Update RazorExtensions.cs

79e023c

captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates and removed area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework labels Mar 17, 2021

captainsafia marked this pull request as ready for review March 17, 2021 22:44

captainsafia requested review from BrennanConroy, halter73, javiercn, pranavkm and a team as code owners March 17, 2021 22:44

captainsafia removed request for a team and halter73 March 17, 2021 22:44

captainsafia removed request for javiercn and BrennanConroy March 17, 2021 22:44

captainsafia commented Mar 17, 2021

View reviewed changes

pranavkm reviewed Mar 17, 2021

View reviewed changes

captainsafia closed this Mar 18, 2021

captainsafia deleted the safia/update-sdk-one-asm branch June 11, 2021 02:27

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

		<AddRazorSupportForMvc>true</AddRazorSupportForMvc>
		<NoWarn>RS0016</NoWarn>

		@@ -0,0 +1,4 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

		builder.Features.Add(new RazorPageDocumentClassifierPass(useConsolidatedMvcViews: true));
		builder.Features.Add(new MvcViewDocumentClassifierPass(useConsolidatedMvcViews: true));

Update SDK to bring in RSG one assembly changes #30827

Update SDK to bring in RSG one assembly changes #30827

Uh oh!

Conversation

captainsafia commented Mar 11, 2021

Uh oh!

captainsafia commented Mar 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captainsafia commented Mar 18, 2021

Uh oh!

Uh oh!